-
-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple backup storage backends #235 #357
Multiple backup storage backends #235 #357
Conversation
Hey @mjlabe, thanks for starting to work on this. From what I gather on the two issues it seems like it would be more desirable to have these defined in a dictionary as described in #61. That way you could do something akin to It seems like this would give the most flexibility. If you want to backup at both locations at the same time you'd need to run the backup command twice but with different storages. |
@jonathan-s Better? I still need to figure out how to unit test it. If I just update the settings in the test to have 2 local storages, would that be good enough? Example:
|
Hello @jonathan-s @mjlabe To give you more context, when I had this idea, Otherwise, great job! |
I just haven't familiarized myself enough with the code base/unit tests yet to verify the backup is created/restored. |
Yeah, that's better. However, we also need to take into account that each storage has different options. So I would say that the following structure would be more apt.
|
And that's where it becomes a significant pain. I can't even find those settings in the source code. Aren't they defined in settings for the specific library (i.e. S3_Bucket_Name and not DBBackup_Bucket_Name)? I don't know of a way to override Django settings dynamically while it's running. Edit: OK. I found it in the docs. I just need to set options in the storage class like I'm setting the storage class.
That should be fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a potential problem. Are you overwriting the options in the Storage
class __init__
regardless of what options were passed?
If I removed that line (64), all unit tests still pass, so what is the purpose?
|
Ok. updated to add global options to Storage options if it does not already exist (to prevent overwriting local setting). I will work on unit tests soon. |
I believe it used to be like that, but that's no longer the case. |
You really never should have to do this. If we have a need for that, then the design is most likely somehow wrong. |
Just an update. I have not abandoned this PR, but I've been sick since the week I opened the PR. I hope to get back to it soon. |
I hope you get well soon!
…Sent from my mobile
On Wed, 11 Nov 2020, 17:17 mjlabe, ***@***.***> wrote:
Just an update. I have not abandoned this PR, but I've been sick since the
week I opened the PR. I hope to get back to it soon.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#357 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQGYESJH6BK6QUR7CXVGXDSPK2H7ANCNFSM4SB5I5BQ>
.
|
Started working again, but I'm getting this running unit tests on master:
I assume the first message is part of the tests, but I should I be getting the second? All tests pass, but I wanted to make sure. |
I will start working on restore tests, but Is this enough testing for backup? Is there a way to test S3? |
@mjlabe Sorry for the late reply here. Yeah, it looks likes some of the older tests can be improved. Not sure exactly what is happening there and if the 'errors' are expected or not. But it looks a bit omnious. As for your tests I'd say that as long as you test that you get the correct configuration I'd say you are golden. You shouldn't need to test that everything is working end to end. |
…when setting storage option in command
Ok. I've written restore tests and added more backup tests to verify the storage configuration in the handle. Please let me know if you find any issues, want changes, or want more tests. Otherwise, this PR is ready. |
Hello @mjlabe! I am the new maintainer for this repo. Looks like this PR has gone stale. Would you like to remerge with the master branch, or would you rather I close this PR? |
Thanks for looking, but I honestly don't remember anything about this, so I will close it. Thanks! Edit: Looks like I can't,so please close it. |
I have started adding the ability to have a "fallback" (i.e. 2nd restore location) for backups. I still need to finish testing, but I wanted to get feedback before I finish. Do we want a single "fallback" option like this, or do we want to support N backup options (like suggested in #61)? Updates to allow for N will be much more difficult. (closes #235 )